Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix record.update by making record.insert act consistently #1669

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

yannham
Copy link
Member

@yannham yannham commented Oct 9, 2023

The issue

Although this has been hidden by polymorphic contracts, record.insert has been subtly broken. Most function - if not all - of the stdlib, by design, ignore empty optional fields. This is in particular the case of record.remove and record.has_field. However, record.insert doesn't follow the same convention, for some reason, and fail to insert a value if there's already a field, even if the latter is an empty optional field.

Thus, when defining e.g. record.update, we end up with the following surprising result:

nickel> let my_insert = fun field content r =>
        let r =
          if %has_field% field r then
            %record_remove% field r
          else
            r
        in
        %record_insert% field r content

nickel> let foo | {value | optional, ..} = {bar = "hello"}
nickel> my_insert "value" "world" foo
error: record_insert: tried to extend a record with the field value, but it already exists
  ┌─ <repl-input-2>:8:9
  │
8 │         %record_insert% field r content
  │         ^^^^^^^^^^^^^^^^^^^^^^^ here

Indeed, has_field doesn't see the empty optional field, but %record_insert% does. This is currently mitigated for std.record.insert because polymorphic contracts generates "spurious" values (contract application that, if ever evaluated, would raise a missing field definition), and thus are properly considered by has_field and removed. However, for an upcoming change (contract elision for static type annotation), the polymorphic contract is elided and the behavior above will be triggered from the normal record.update.

Content

I started by making variants of has_field and record_remove that are able to see through empty optional fields. Only then I a realized that the actual issue wasn't that has_field was ignoring empty optionals, but rather than record_insert was the only primop to not do so. Still, I think those variants will prove useful, as I believe we will have demand for the corresponding functions in the stdlib - that is, functions that are able to see and manipulate empty optionals. I propose to let the code stay, even if it's not strictly required for the issue addressed by this PR.

The heart of the fix is then just to make record_insert to be

Backward compatiblity

This makes record.insert succeeds on more programs, as it would refuse to insert something if there was an empty optional already before this change. Now it has the same behavior on previous cases, but it doesn't fail when there is a non-empty optional field already.

@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 18:10 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 18:25 Inactive
Copy link
Member

@jneem jneem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about the naming, because for example it sounds to me like record_insert_all will insert multiple elements into the record. But I don't have a better suggestion right now. If we want to change it later, does renaming primops count as a breaking change, or do we consider them private?

core/src/term/mod.rs Outdated Show resolved Hide resolved
@yannham
Copy link
Member Author

yannham commented Oct 9, 2023

I'm not crazy about the naming, because for example it sounds to me like record_insert_all will insert multiple elements into the record. But I don't have a better suggestion right now. If we want to change it later, does renaming primops count as a breaking change, or do we consider them private?

I entirely agree, and I proceeded nonetheless for this exact reason: primops aren't part of the stability guarantee, so we can rename them as we see fit.

Co-authored-by: jneem <joeneeman@gmail.com>
@github-actions github-actions bot temporarily deployed to pull request October 9, 2023 19:03 Inactive
@yannham yannham added this pull request to the merge queue Oct 10, 2023
Merged via the queue into master with commit 94c2573 Oct 10, 2023
5 checks passed
@yannham yannham deleted the fix/record_insert branch October 10, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants